Skip to content

IDP: avoid logging value of 'idp_client_secret' #8332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
alexey-tikhonov merged 3 commits into from
Jan 13, 2026
Merged

IDP: avoid logging value of 'idp_client_secret' #8332

alexey-tikhonov merged 3 commits into from
Jan 13, 2026

Conversation

@alexey-tikhonov
Copy link
Member

@alexey-tikhonov alexey-tikhonov commented Jan 7, 2026

No description provided.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Jan 7, 2026
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jan 7, 2026
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to prevent logging the sensitive idp_client_secret value. The changes introduce a specific check in dp_get_options to avoid printing the secret's value, which is a good security improvement. The PR also refactors the debug logging level for option processing to a more appropriate level and consistently uses a macro for the secret's key. However, the fix is incomplete as a similar logging vulnerability remains in another function within the same file, which could still lead to secret leakage. This critical issue needs to be addressed.

@alexey-tikhonov alexey-tikhonov added the Trivial A single reviewer is sufficient to review the Pull Request label Jan 7, 2026
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review January 7, 2026 21:09
@sumit-bose
Copy link
Contributor

sumit-bose commented Jan 8, 2026

Hi,

thank you for the patch. Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Jan 8, 2026

Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

I did.
But IIUC this would allow to configure non-ascii characters in the value and wasn't sure if 'oidc_child' would need additional filtering then.

@sumit-bose
Copy link
Contributor

sumit-bose commented Jan 8, 2026

Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

I did. But IIUC this would allow to configure non-ascii characters in the value and wasn't sure if 'oidc_child' would need additional filtering then.

I thought it is only about \0 in the value, DP_OPT_STRING would allow non-ascii like UTF-8 as well?

While talking about oidc_child, can you fix the logging in read_client_secret_from_stdin() as well?

Thanks.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Jan 8, 2026

Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

I did. But IIUC this would allow to configure non-ascii characters in the value and wasn't sure if 'oidc_child' would need additional filtering then.

I thought it is only about \0 in the value, DP_OPT_STRING would allow non-ascii like UTF-8 as well?

The questions is: what should be the type of this option from idp-provider/oidc_child point of view?
I thought it was 'string', not 'blob'.
If that's the case I still think that dp_get_options() needs to be fixed, not type of the option changed just because this would change logging behavior of some utility function (this is fragile - nothing will prevent changing this utility function to log blob values as well later).

A better solution would be introduction of a new 'DP_OPT_SENSITIVE_STRING' that would have proper d-tor and serve as a reminder "do not log me", but this would be much more invasive.

As to ldap_default_authtok, I thought it has BLOB type because it can have type 'obfuscated_password' and this is really a set of random bytes, no? I might be wrong here.

While talking about oidc_child, can you fix the logging in read_client_secret_from_stdin() as well?

Done.

@alexey-tikhonov alexey-tikhonov force-pushed the dont-log-secrets branch 2 times, most recently from ddfa1e1 to 8242fb2 Compare January 12, 2026 10:03
@sumit-bose sumit-bose added the coverity Trigger a coverity scan label Jan 12, 2026
@sumit-bose
Copy link
Contributor

sumit-bose commented Jan 12, 2026

Coverity is green.

@sumit-bose sumit-bose removed the coverity Trigger a coverity scan label Jan 12, 2026
@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Jan 13, 2026

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jan 13, 2026
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses a critical security vulnerability by preventing the idp_client_secret from being logged. The changes are comprehensive, covering logging during configuration loading, when reading the secret from stdin, and when including it in POST request data. The introduction of the log_string_option function is a clean way to handle sensitive configuration options, and replacing the hardcoded string "idp_client_secret" with the CONFDB_IDP_CLIENT_SECRET macro improves code maintainability. The changes appear correct and complete. Well done.

sumit-bose
sumit-bose approved these changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the fixes, ACK.

bye,
Sumit

@alexey-tikhonov @sssd-bot
DP: use 'SSSDBG_CONF_SETTINGS' to log options
ec88ee4 
Reviewed-by: Sumit Bose <sbose@redhat.com>
@alexey-tikhonov @sssd-bot
IDP: avoid logging value of 'idp_client_secret'
43db9b5 
Note that 'ldap_default_authtok' doesn't require special handling
because it is of DP_OPT_BLOB type and isn't logged.

Reviewed-by: Sumit Bose <sbose@redhat.com>
@alexey-tikhonov @sssd-bot
OIDC_CHILD: don't log 'post_data' content
b291944 
as it might contain 'secret'

Reviewed-by: Sumit Bose <sbose@redhat.com>
@sssd-bot
Copy link
Contributor

sssd-bot commented Jan 13, 2026

The pull request was accepted by @sumit-bose with the following PR CI status:

🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🔴 ci / system (fedora-44) (failure)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@alexey-tikhonov alexey-tikhonov merged commit 44b938a into SSSD:master Jan 13, 2026
11 of 15 checks passed
@alexey-tikhonov alexey-tikhonov mentioned this pull request Jan 22, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sumit-bose sumit-bose sumit-bose approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sumit-bose sumit-bose

Labels

Accepted no-backport This should go to target branch only. Trivial A single reviewer is sufficient to review the Pull Request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexey-tikhonov @sumit-bose @sssd-bot